-
Notifications
You must be signed in to change notification settings - Fork 121
[POS as a tab i2] Streamline loading UI animation when transitioning from eligibility check to items loading #15922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POS as a tab i2] Streamline loading UI animation when transitioning from eligibility check to items loading #15922
Conversation
…to `PointOfSaleDashboardView` to enable continuous loading animation. UI state is determined by `PointOfSaleDashboardViewHelper`.
|
|
iamgabrielma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚢
| @available(iOS 17.0, *) | ||
| @Test func handleTap_when_attempt_to_add_duplicated_coupons_in_list_then_does_not_add_it_to_cart() async throws { | ||
| let aggregateModel = PointOfSaleAggregateModel(itemsController: MockPointOfSaleItemsController(), | ||
| let aggregateModel = PointOfSaleAggregateModel(entryPointController: POSEntryPointController(eligibilityChecker: MockPOSEligibilityChecker()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can make a func makeAggregate() -> PointOfSaleAggregateModel helper in this test file, all cases seem to DI the same dependency into the SUT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running the tests locally, I encountered memory crashes with/without the improvements in ea85627 sometimes:
#0 0x0000000104c88874 in __pthread_kill ()
#1 0x000000010437a2ec in pthread_kill ()
#2 0x0000000180171ea8 in abort ()
#3 0x00000001801f4450 in malloc_vreport ()
#4 0x00000001801f4610 in malloc_report ()
#5 0x00000001801c3708 in ___BUG_IN_CLIENT_OF_LIBMALLOC_POINTER_BEING_FREED_WAS_NOT_ALLOCATED ()
#6 0x00000001803a7270 in -[__NSArrayM insertObject:atIndex:] ()
#7 0x000000010280d46c in +[WPAnalytics registerTracker:] at /Users/jaclynchen/Developer/woocommerce-ios/Modules/Sources/WordPressSharedObjC/Analytics/WPAnalytics.m:25
#8 0x000000010ba93674 in WooAnalytics.init(analyticsProvider:) at /Users/jaclynchen/Developer/woocommerce-ios/WooCommerce/Classes/Analytics/WooAnalytics.swift:43
#9 0x000000010ba9358c in WooAnalytics.__allocating_init(analyticsProvider:) ()
#10 0x0000000133db65fc in default argument 7 of makePointOfSaleAggregateModel(entryPointController:itemsController:purchasableItemsSearchController:couponsController:couponsSearchController:cardPresentPaymentService:orderController:analytics:collectOrderPaymentAnalyticsTracker:searchHistoryService:popularPurchasableItemsController:barcodeScanService:soundPlayer:paymentState:) at /Users/jaclynchen/Developer/woocommerce-ios/WooCommerce/WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift:1020
#11 0x0000000133dba018 in PointOfSaleAggregateModelTests.OrderStageTests.addMoreToCart_moves_to_building_order_stage() ()
#12 0x0000000133db9c58 in static PointOfSaleAggregateModelTests.OrderStageTests.$s16WooCommerceTests025PointOfSaleAggregateModelC0V010OrderStageC0V43addMoreToCart_moves_to_building_order_stage4TestfMp_9Z7114aedefMu_@Sendable () at /var/folders/dq/w2cq_v314913sdrjxwl5881c0000gn/T/swift-generated-sources/@__swiftmacro_16WooCommerceTests025PointOfSaleAggregateModelC0V010OrderStageC0V43addMoreToCart_moves_to_building_order_stage4TestfMp_.swift:7
But I didn't see any previous mentions from MGS, nor from CI failures. If it happens on CI, I'll revert the commit and we can look into improvements in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good on CI, merging now.
| @available(iOS 17.0, *) | ||
| @Test func handleTap_when_attempt_to_add_duplicated_coupons_in_list_then_does_not_add_it_to_cart() async throws { | ||
| let aggregateModel = PointOfSaleAggregateModel(itemsController: MockPointOfSaleItemsController(), | ||
| let aggregateModel = PointOfSaleAggregateModel(entryPointController: POSEntryPointController(eligibilityChecker: MockPOSEligibilityChecker()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can make a func makeAggregate() -> PointOfSaleAggregateModel helper in this test file, all cases seem to DI the same dependency into the SUT.
| @Test(arguments: [ | ||
| PointOfSaleErrorState.errorOnLoadingProducts(), | ||
| PointOfSaleErrorState.errorOnLoadingVariations(), | ||
| PointOfSaleErrorState.errorOnLoadingCoupons(), | ||
| PointOfSaleErrorState.errorCouponsDisabled, | ||
| PointOfSaleErrorState.errorOnLoadingProductsNextPage(), | ||
| PointOfSaleErrorState.errorOnLoadingVariationsNextPage(), | ||
| PointOfSaleErrorState.errorOnLoadingCouponsNextPage(), | ||
| PointOfSaleErrorState.errorOnRefreshingCoupons(), | ||
| PointOfSaleErrorState.errorOnEnablingCoupons() | ||
| ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice SwiftTesting 💯
| // MARK: - Horizontal Size Class Tests | ||
|
|
||
| @available(iOS 17.0, *) | ||
| @Test func determineViewState_when_horizontalSizeClass_is_compact_returns_unsupportedWidth() async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting to see that with this approach we can now unit test unsupported widths, nice!
| case .coupons(search: true): | ||
| await posModel.couponsSearchController.loadItems(base: .root) | ||
| } | ||
| switch viewState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much cleaner view hierarchy 💯
…te factory method with mocks as default.

For WOOMOB-767
Just one review is required.
Why
For stores eligible for POS, when transitioning from eligibility check to items loading, both being async tasks, the spinner animation appears discontinuous during the transition.
How
This PR streamlines the animation by moving the eligibility state based UI from a separate view (entry point view) to the dashboard view with some refactoring on the view state. Other attempts on the loading UI animation side that did not work due to having 3 sets of animations using SwiftUI
withAnimationwhich is reset when the view is redrawn (triggered when enteringPointOfSaleDashboardView):IndefiniteCircularProgressViewStyleto only start the timer if there is no timer before, and disable invalidating the timer inonDisappear.withAnimationentirely usingCADisplayLinktimer - I could not get it to match the previous animation 100%.Description
This PR moves the eligibility state based UI from
PointOfSaleEntryPointViewtoPointOfSaleDashboardViewand refactorsPointOfSaleEntryPointViewview body to use a centralizedViewStateapproach with a view helper class, improving code maintainability and following the POS architecture guidelines.Key Changes:
PointOfSaleDashboardViewHelperPointOfSaleDashboardView, like updating the floating control visibility and adding the frame width to fit to parent widthArchitecture Benefits:
Steps to reproduce
Prerequisite: the store is in an eligible country (US/UK) and is one fixable reason away from being eligible for POS (e.g. unsupported currency, POS feature switch disabled for WC version 10.0+)
Retry--> it should enter loading UI to fetch products, and to the products loaded stateTesting information
Tested on iPad Pro 11in 3rd generation device, iOS 18.5:
Screenshots
Ineligible -> eligible with split screen testing:
ineligible-to-eligible.mp4
Eligible where the animation is now continuous:
eligible.mp4
RELEASE-NOTES.txtif necessary.